Improve RangeChip with tagged table#38
Conversation
han0110
force-pushed
the
feature/range-chip-with-tagged-table
branch
from
46eb3f1 to
4505bf1
Compare
CPerezz
left a comment
CPerezz
left a comment
There was a problem hiding this comment.
LGTM!
I just left some suggestions that we might want to adopt. Up to you :)
Feel free to address what you think it's fine to and merge!
Thanks for this great work! 🎉
| let config = TableConfig { | ||
| selector: meta.complex_selector(), | ||
| column: meta.lookup_table_column(), | ||
| let (s_overflow, tag_overflow) = if has_overflow_limb { |
There was a problem hiding this comment.
Maybe? Just a suggestion to save the creation of a variable that might be avoidable. I say it more for ease of read than performance of course.
| let (s_overflow, tag_overflow) = if has_overflow_limb { | |
| let (s_overflow, tag_overflow) = if !overflow_bit_lens.is_empty() { |
| macro_rules! lookup { | ||
| ($prefix:literal, $selector:ident, $tag:ident, $bit_lens:ident, [$($value:ident),*]) => { | ||
| $( | ||
| meta.lookup(concat!($prefix, stringify!($value)), |meta| { | ||
| let selector = meta.query_selector($selector); | ||
| let tag = match $tag { | ||
| Some(tag) => meta.query_fixed(tag, Rotation::cur()), | ||
| None => { | ||
| let tag = F::from(bit_len_tag[&$bit_lens[0]] as u64); | ||
| selector.clone() * Expression::Constant(tag) | ||
| }, | ||
| }; | ||
| let value = meta.query_advice($value, Rotation::cur()); | ||
| vec![(tag, t_tag), (selector * value, t_value)] | ||
| }); | ||
| )* | ||
| }; | ||
| } |
There was a problem hiding this comment.
Generally I try to avoid macros. But this one in particular is clear, easy to understand and with a really tiny scope. So it's fine at least for me to keep it as is.
There was a problem hiding this comment.
Anyway, feels like it could be replaced by a function or closure.
| use std::marker::PhantomData; | ||
|
|
||
| /// `PointRepresentation` will encode point with an implemented strategy | ||
| /// TODO: Generalize `ecc_chip` with `EccInstrucitons` |
There was a problem hiding this comment.
Should we file an issue for this and remove the TODO?
There was a problem hiding this comment.
Already filed the issue. Feel free to just remove this.
|
Feel free to merge when all the checks pass @han0110 🎉 |
2 participants
Use tagged table in
RangeChipto reduce number of lookup.Resolves #37